refactor(orgs): get organizations via GraphQL and display name instead login by lex111 · Pull Request #347 · gitpoint/git-point

lex111 · 2017-09-20T14:20:04Z

There are currently two requests when receiving organizations: one requests public organizations, another request also requests organizations with private membership. Using GraphQL, we can get all organizations with one query. However, doing so required updating the scope of the token. We need to somehow come up with a way to update the token, how to do it better, who has any ideas?

Also, within the framework of this PR done that, the organization name is output instead of the login, this closes #272

jouderianjr · 2017-09-20T15:47:38Z

+  return response.json().then(data => {
+    const orgs = [];
+
+    data.data.user.organizations.edges.reduce((key, item) =>


Hey, I think that's not the best way of use reduce method, reduce returns something, this way you are just iterating over the array(similar what we do with forEach)

What do you think about this approach?

return data.data.user.organizations.edges .map(item => item.node) .sort((org1, org2) => org1.name > org2.name)

@jouderianjr I completely agree, thank you, I will fix it.

lex111 · 2017-09-20T17:56:17Z

There remains such a problem: we need to open a link to reauthorize, in order for the user to update the token. How is this best done?

machour · 2017-09-23T18:53:02Z


 export async function fetchUserOrgs(user, accessToken) {
-  const ORGS_ENDPOINT = `${root}/users/${user}/orgs`;
+  const getOrganizationsQuery = `


I'm a bit scared of seeing api/index.js grow too much. Would it be possible to put queries in entities subfolders ? This one for example would go in user/user.queries.js

I agree 👍

Highly agree, I made the mistake of laying things all down in api/index.js but it really makes sense to leverage feature api files (like we do with other parts of the app).

What do you think about moving this query into something like

/src/queries/foo.query.js or something like that?

Maybe it's better this way /src/api/queries/foo.query.js?

What about /src/user/user.queries.js ? (just like we'll have user.api.js) ?

machour · 2017-09-23T18:55:26Z

                style={styles.avatar}
                source={{
-                  uri: user.avatar_url,
+                  uri: imageUrl || user.avatar_url,


I think it's best to make the imageUrl prop mandatory and pass it as { user.avatar_url } where needed

I do not think that it will turn out, i.e. when querying GraphQL, another notation is used (lowerCase instead of snake_case). We have already done this here.

I still think that having an optional prop, magically provided by another prop in some cases, is a bit confusing. I'm in favor of "real dumb" presentation components.

Also in the spirit of #350 (did you check it?), I think GraphQL responses should be stored using the same schemas as REST responses in the entities array. That would enable us to access the data in the same way, regardless of where it came from (rest or graphql). Am I making sense? :/

I have not watched, but probably this will solve the problem, it seems to me...

housseindjirdeh

I'm loving this @lex111, thank you. Left some discussion comments

housseindjirdeh · 2017-09-23T22:37:49Z

  signIn = () =>
    openURLInView(
-      `https://github.com/login/oauth/authorize?response_type=token&client_id=${CLIENT_ID}&redirect_uri=gitpoint://welcome&scope=user%20repo&state=${stateRandom}`
+      `https://github.com/login/oauth/authorize?response_type=token&client_id=${CLIENT_ID}&redirect_uri=gitpoint://welcome&scope=user%20repo%20read:org&state=${stateRandom}`


Looks like this adds read access to org data. Without this, does your queries not work?

Yes, without this, not getting an organization, there will be an error. Therefore, I write that there is a problem: we need to update the token when changing the scope, as for example in this case. And the question: how to do this?

housseindjirdeh · 2017-09-23T22:49:38Z


 export async function fetchUserOrgs(user, accessToken) {
-  const ORGS_ENDPOINT = `${root}/users/${user}/orgs`;
+  const getOrganizationsQuery = `


Highly agree, I made the mistake of laying things all down in api/index.js but it really makes sense to leverage feature api files (like we do with other parts of the app).

housseindjirdeh · 2017-09-23T22:52:05Z

+                id
+                login
+                name
+                avatarUrl


Seconding @machour's comment. I still don't know how exactly GraphQL works, but can we not stick to the same mention of avatar_url or does this version of the API just return it as avatarUrl instead of avatar_url?

That's just the point, that the GraphQL query returns the keys in another notation

FWIW I think you can make an alias to what you get from GQL. I'd prefer camelCase instead of snake_case though :P

machour · 2017-09-28T10:22:00Z

@lex111 For your changed scope and the need to re-authorize, I think this call should help:
https://developer.github.com/v3/oauth_authorizations/#check-an-authorization
You can see with this call if your needed permission is granted, or if you should ask the user to grant it

alejandronanez · 2017-09-28T10:46:08Z

                  <WebView
                    source={{
-                      uri: `https://github.com/login/oauth/authorize?response_type=token&client_id=${CLIENT_ID}&redirect_uri=gitpoint://welcome&scope=user%20repo&state=${stateRandom}`,
+                      uri: `https://github.com/login/oauth/authorize?response_type=token&client_id=${CLIENT_ID}&redirect_uri=gitpoint://welcome&scope=user%20repo%20read:org&state=${stateRandom}`,


Maybe for later but I think we can move this url into a constants files.

machour · 2018-04-23T10:10:55Z

Closing this PR as it's superseded by the ongoing store refactoring. #272 remains open and will be dealt with.

lex111 added 2 commits September 20, 2017 16:45

refactor(orgs): get organizations via GraphQL

78ef77e

feat(oauth): add read org scope to GitHub OAuth

f3a5ee5

lex111 added the help wanted label Sep 20, 2017

lex111 force-pushed the refactor-get-orgs branch from 551ee53 to 2a92c3b Compare September 20, 2017 14:37

feat(orgs): made name display instead of login

16c27ee

lex111 force-pushed the refactor-get-orgs branch from 2a92c3b to 16c27ee Compare September 20, 2017 14:37

jouderianjr reviewed Sep 20, 2017

View reviewed changes

refactor(fetchUserOrgs): using map instead reduce

fe77ed0

lex111 requested review from alejandronanez, andrewda, housseindjirdeh and machour September 23, 2017 18:23

machour requested changes Sep 23, 2017

View reviewed changes

housseindjirdeh reviewed Sep 23, 2017

View reviewed changes

andrewda force-pushed the master branch from 0751df7 to cb06e0d Compare September 23, 2017 23:31

lex111 mentioned this pull request Sep 26, 2017

refactor(api): Api clean up #361

Merged

Merge branch 'master' into refactor-get-orgs

70a8496

alejandronanez approved these changes Sep 28, 2017

View reviewed changes

housseindjirdeh mentioned this pull request Sep 28, 2017

v1.3.0 Prep #381

Closed

housseindjirdeh mentioned this pull request Oct 8, 2017

GraphQL #141

Open

lex111 mentioned this pull request Oct 9, 2017

wip: use GraphQL API to fetch issues and pull requests #455

Closed

machour mentioned this pull request Oct 24, 2017

Discussion - Move to Redux Actions + Reselect #520

Closed

machour closed this Apr 23, 2018

chinesedfan added the graphql label Oct 30, 2018

Conversation

lex111 commented Sep 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lex111 commented Sep 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lex111 Sep 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

housseindjirdeh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

machour commented Sep 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

machour commented Apr 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lex111 Sep 23, 2017 •

edited

Loading